codeframe/ui/routers/terminal_ws.py (1)

100-103: Consider handling paused session state explicitly.

Per the schema in codeframe/persistence/schema_manager.py, valid states are active, paused, and ended. Currently, only ended is blocked. Consider whether terminal access should also be blocked for paused sessions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@codeframe/ui/routers/terminal_ws.py` around lines 100 - 103, The session
check currently only blocks "ended" sessions; update the conditional in the
terminal WebSocket connection handling (where session is fetched via
db.interactive_sessions.get using session_id) to also treat "paused" as
disallowed—i.e., change the if to verify session is not None and that
session.get("state") is not in ("ended", "paused") or equivalently check if
session.get("state") in ("ended", "paused") then await websocket.close(... ) and
return; ensure you reference the same websocket.close call and session/state
keys so paused sessions are blocked like ended ones.

web-ui/src/components/sessions/AgentTerminal.tsx (2)

38-51: Consider using Hugeicons instead of inline SVG.

Per coding guidelines, the web UI should use @hugeicons/react rather than custom SVG icons. Check if Hugeicons provides a suitable loading spinner icon.

As per coding guidelines: "Web UI must use Shadcn/UI (Nova preset) with gray color scheme and Hugeicons (@hugeicons/react); never use lucide-react"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/components/sessions/AgentTerminal.tsx` around lines 38 - 51,
Replace the inline SVG spinner in AgentTerminal (the <svg ...> block rendering
the loading spinner) with the appropriate Hugeicons component from
`@hugeicons/react` (e.g., the library's spinner/loading icon), import that icon at
the top of the file and use it where the current svg is rendered, apply the
component the same sizing/animation classes but switch to the gray color scheme
(e.g., "animate-spin h-5 w-5 text-gray-400") to follow the Shadcn/UI Nova
preset, and remove the inline SVG so no custom SVG or lucide-react icon remains.

---

77-80: Setting ref during render is unconventional.

While this works for the initial mount, it's a side effect during render. Consider moving this initialization to a useMemo or early in the useEffect for clarity.

♻️ Alternative using useMemo

+import { useEffect, useRef, useMemo } from 'react';
 
-  const wsUrlRef = useRef<string | null>(null);
-
-  // Build the WS URL once per sessionId (requires client side)
-  if (typeof window !== 'undefined' && !wsUrlRef.current) {
-    wsUrlRef.current = buildWsUrl(sessionId);
-  }
+  const wsUrl = useMemo(() => {
+    if (typeof window === 'undefined') return null;
+    return buildWsUrl(sessionId);
+  }, [sessionId]);

   const { status, sendInput, sendResize } = useTerminalSocket({
-    url: wsUrlRef.current,
+    url: wsUrl,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/components/sessions/AgentTerminal.tsx` around lines 77 - 80, In
AgentTerminal, setting wsUrlRef.current during render is a side effect; move the
initialization out of render by computing the value with useMemo (e.g., const
wsUrl = useMemo(() => buildWsUrl(sessionId), [sessionId]) and assign to
wsUrlRef.current inside a useEffect) or by assigning wsUrlRef.current at the
start of a useEffect that depends on sessionId; update references that currently
read wsUrlRef.current to use the memoized wsUrl or ensure the ref is set in the
effect to avoid performing side effects during render.

web-ui/src/hooks/useTerminalSocket.ts (2)

83-89: Consider caching the TextEncoder instance.

A new TextEncoder is instantiated on every string message. Since TextEncoder is stateless, it can be created once at module scope or as a constant outside the hook.

♻️ Proposed optimization

 import { useEffect, useRef, useCallback, useState } from 'react';
 
+const textEncoder = new TextEncoder();
+
 export type TerminalSocketStatus = 'idle' | 'connecting' | 'open' | 'closed' | 'error';

Then update the usage:

       } else if (typeof event.data === 'string') {
-        onDataRef.current(new TextEncoder().encode(event.data));
+        onDataRef.current(textEncoder.encode(event.data));
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/hooks/useTerminalSocket.ts` around lines 83 - 89, Cache a single
TextEncoder instance instead of instantiating a new one for every string
message: create one TextEncoder at module scope or as a const outside the hook
and use it inside the ws.onmessage handler (the current block referencing
ws.onmessage, event.data, and onDataRef.current). Replace new
TextEncoder().encode(event.data) with the cached encoder.encode(event.data) so
string messages reuse the same encoder.

---

5-24: Consider moving exported types to the central types file.

The exported types TerminalSocketStatus, UseTerminalSocketOptions, and UseTerminalSocketReturn are defined inline. Per project guidelines, TypeScript types should be defined in web-ui/src/types/index.ts and re-imported here.

As per coding guidelines: "TypeScript types must be defined in web-ui/src/types/index.ts"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/hooks/useTerminalSocket.ts` around lines 5 - 24, Move the exported
type declarations TerminalSocketStatus, UseTerminalSocketOptions, and
UseTerminalSocketReturn out of this file and into the central types barrel
(web-ui/src/types/index.ts), then import and re-export or use them here;
specifically, create/type-add these three symbols in the central types file with
the same definitions, update this module to remove the inline definitions and
import { TerminalSocketStatus, UseTerminalSocketOptions, UseTerminalSocketReturn
} from the central types entry, and ensure any existing exports or usages
(sendInput, sendResize, status) still reference the imported types.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

web-ui/src/__tests__/hooks/useTerminalSocket.test.ts (1)

105-107: ⚠️ Potential issue | 🟡 Minor

The test will fail due to reference inequality in the assertion.

The simulateBinaryMessage() passes bytes.buffer to the onmessage handler. The hook then creates a new Uint8Array from this buffer internally. Since Jest's toHaveBeenCalledWith() uses reference equality, the assertion will fail.

🔧 Proposed fix to compare array contents

     const bytes = new Uint8Array([104, 101, 108, 108, 111]); // "hello"
     act(() => ws.simulateBinaryMessage(bytes));
-    expect(onData).toHaveBeenCalledWith(bytes);
+    expect(onData).toHaveBeenCalledTimes(1);
+    expect(Array.from(onData.mock.calls[0][0])).toEqual([104, 101, 108, 108, 111]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 105 - 107,
The test fails because simulateBinaryMessage passes bytes.buffer and the hook
creates a new Uint8Array, so reference equality fails; update the assertion for
the onData mock used by useTerminalSocket to compare contents instead of
references (e.g., assert the received value is a Uint8Array and use toEqual
against the original bytes or compare onData.mock.calls[0][0] to bytes) — locate
the simulateBinaryMessage call and the onData mock in the test and replace
expect(onData).toHaveBeenCalledWith(bytes) with a content-based equality check
for the Uint8Array.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

web-ui/__tests__/components/tasks/TaskBoardView.test.tsx (1)

245-245: Indentation inconsistency on mock setup lines.

These 6 lines are inside it() callbacks but have no leading indentation, while the rest of the test body is properly indented with 4 spaces. The code works correctly, but inconsistent formatting affects readability.

🔧 Example fix (apply same pattern to all 6 lines)

   it('calls stopExecution and mutates when Stop is clicked', async () => {
-tasksApi.stopExecution.mockResolvedValue(undefined);
+    tasksApi.stopExecution.mockResolvedValue(undefined);
     mockMutate.mockResolvedValue(undefined);

Also applies to: 259-259, 273-273, 323-323, 364-364, 389-389

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/__tests__/components/tasks/TaskBoardView.test.tsx` at line 245,
Several mock setup lines (e.g., tasksApi.stopExecution.mockResolvedValue(...))
inside it() callbacks are not indented to match the rest of the test bodies; fix
by adding the same 4-space indentation as the surrounding test code so those
lines align with the rest of the it() block. Update each occurrence (including
the instances at the positions referenced: the
tasksApi.stopExecution.mockResolvedValue line and the other similar
mockResolvedValue lines noted) to use the consistent 4-space indentation used
throughout TaskBoardView.test.tsx.

web-ui/src/__tests__/hooks/useTerminalSocket.test.ts (3)

111-122: Assertion style inconsistent with binary frame test.

The binary frame test (lines 107-108) uses Array.from() comparison to avoid reference equality issues, but this test still relies on toHaveBeenCalledWith(expected). While modern Jest performs deep equality on typed arrays, applying the same pattern as the binary test would ensure consistency and avoid potential fragility.

♻️ Suggested fix for consistency

     act(() => ws.simulateTextMessage('hi'));
-    const expected = new TextEncoder().encode('hi');
-    expect(onData).toHaveBeenCalledWith(expected);
+    const received = onData.mock.calls[0][0] as Uint8Array;
+    // 'hi' encodes to [104, 105]
+    expect(Array.from(received)).toEqual([104, 105]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 111 - 122,
The test in useTerminalSocket.test.ts should avoid direct typed-array reference
equality by mirroring the binary-frame test: when asserting the onData call
after MockWebSocket.simulateTextMessage('hi'), convert the received arg and the
expected TextEncoder().encode('hi') to plain arrays (e.g., via Array.from) and
assert equality (use the same Array.from pattern as used in the binary-frame
test) so you compare values rather than references for the onData mock call.

---

74-217: Consider adding test coverage for WebSocket error events.

The test suite covers the core functionality well, but there's no test for the onerror handler behavior. If the hook handles errors differently from connection closes (e.g., immediate error state without retry), that behavior should be validated.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 74 - 217,
Add a test that covers WebSocket error events for useTerminalSocket: identify
the MockWebSocket instance (MockWebSocket.instances[0]), simulate an error
(implement or call a simulateError method on MockWebSocket), and assert the
hook's behavior (e.g., result.current.status becomes 'error' immediately or
follows retry logic) and that reconnects/cleanup behave as expected (respecting
maxRetries/retryDelay or no retries). Ensure the test calls act(() =>
ws.simulateError()) and then advances timers if needed
(jest.advanceTimersByTime) to validate retry behavior or final state; reference
the hook functions sendInput/sendResize only if needed to confirm socket
closed/cleaned up.

---

58-70: Consider restoring original WebSocket in afterEach.

The mock replaces global.WebSocket but never restores it. While Jest typically isolates test files, storing and restoring the original helps prevent interference if test isolation is incomplete.

♻️ Suggested improvement

+let originalWebSocket: typeof WebSocket | undefined;
+
 beforeEach(() => {
   MockWebSocket.instances = [];
+  originalWebSocket = (global as any).WebSocket;
   (global as any).WebSocket = MockWebSocket;
   // jsdom doesn't ship TextEncoder; polyfill from Node
   if (typeof (global as any).TextEncoder === 'undefined') {
     (global as any).TextEncoder = NodeTextEncoder;
   }
   jest.useFakeTimers();
 });

 afterEach(() => {
   jest.useRealTimers();
+  (global as any).WebSocket = originalWebSocket;
 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 58 - 70,
The test replaces global.WebSocket with MockWebSocket but never restores the
original, so modify the setup/teardown around beforeEach/afterEach: in the
beforeEach capture the existing global.WebSocket into a local variable (e.g.,
origWebSocket) before assigning MockWebSocket, and in afterEach restore
global.WebSocket from that saved origWebSocket (and clear
MockWebSocket.instances as needed); reference the existing beforeEach,
afterEach, MockWebSocket and uses of (global as any).WebSocket to locate where
to add the save/restore.

tests/ui/test_terminal_ws.py (2)

41-98: Auth tests cover key rejection scenarios but use overly broad exception catching.

The tests validate missing token, invalid token, session not found, ended session, and ownership mismatch. However, pytest.raises(Exception) is too broad and could mask unrelated failures.

♻️ Consider catching specific WebSocket exceptions

FastAPI's TestClient raises WebSocketDisconnect or similar when the connection is rejected. Using a more specific exception type would make test failures clearer:

+from starlette.websockets import WebSocketDisconnect

 def test_missing_token_closes_4001(self):
     app = _make_app()
     client = TestClient(app)
-    with pytest.raises(Exception):
+    with pytest.raises(Exception):  # WebSocket rejection raises generic Exception in TestClient
         # No token → closed before accepting; TestClient raises on non-101
         with client.websocket_connect("/ws/sessions/s1/terminal"):
             pass

If WebSocketDisconnect is not directly catchable, adding a comment explaining why Exception is used would improve clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/ui/test_terminal_ws.py` around lines 41 - 98, The tests in class
TestTerminalWsAuth use pytest.raises(Exception) which is too broad; update each
test (e.g., test_missing_token_closes_4001, test_invalid_token_closes_4001,
test_valid_token_session_not_found, test_valid_token_ended_session,
test_ownership_mismatch_closes) to assert the specific WebSocket disconnect
exception instead of Exception — import WebSocketDisconnect from
starlette.websockets (or the appropriate WebSocketDisconnect used by TestClient)
and replace pytest.raises(Exception) with pytest.raises(WebSocketDisconnect); if
that specific exception cannot be caught by TestClient in your test environment,
leave the Exception but add a brief comment above the with pytest.raises(...)
explaining why the broad Exception is required.

---

143-170: Test verifies no crash but lacks assertion on behavior.

The test sends a resize message and implicitly passes if no exception is raised. Adding an assertion (e.g., that stdin.write was not called with the resize JSON) would strengthen the test.

💡 Proposed enhancement: assert resize message is not forwarded

         with client.websocket_connect("/ws/sessions/s1/terminal?token=x") as ws:
             # Sending a resize message should not raise
             ws.send_text(json.dumps({"type": "resize", "cols": 80, "rows": 24}))
+            # Verify resize message was NOT written to subprocess stdin
+            # (stdin.write should not have been called with the resize JSON)
+            for call in mock_proc.stdin.write.call_args_list:
+                if call.args:
+                    written = call.args[0]
+                    assert b'"type": "resize"' not in written, "resize should not be forwarded to stdin"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/ui/test_terminal_ws.py` around lines 143 - 170, The test
test_resize_message_does_not_crash should assert that the resize JSON is not
forwarded to the process: after calling
ws.send_text(json.dumps({"type":"resize","cols":80,"rows":24})), add an
assertion that mock_proc.stdin.write (or
mock_proc.stdin.write.assert_not_called()/assert_not_awaited) was not invoked
with the resize payload (or not invoked at all), referencing mock_proc.stdin and
ws.send_text to locate the relevant mocks and call; ensure to use the
appropriate AsyncMock assertion (e.g., assert_not_awaited or assert_not_called)
for the async stdin write.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

codeframe/ui/routers/terminal_ws.py (1)

100-103: ⚠️ Potential issue | 🟠 Major

Session state check should only allow "active" sessions.

The current check rejects only "ended" sessions, but "paused" sessions should also be denied terminal access. Change the condition to explicitly require state == "active".

     session = await asyncio.to_thread(db.interactive_sessions.get, session_id)
-    if session is None or session.get("state") == "ended":
-        await websocket.close(code=4004, reason="Session not found or ended")
+    if session is None:
+        await websocket.close(code=4004, reason="Session not found")
+        return
+    if session.get("state") != "active":
+        await websocket.close(code=4004, reason="Session is not active")
         return

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@codeframe/ui/routers/terminal_ws.py` around lines 100 - 103, The session
validation currently only rejects sessions with state "ended"; update the check
after session = await asyncio.to_thread(db.interactive_sessions.get, session_id)
to require session.get("state") == "active" (i.e., treat any non-"active" state,
including "paused" or missing state, as invalid). Replace the existing condition
(session is None or session.get("state") == "ended") with a negative check
(session is None or session.get("state") != "active") so that
websocket.close(code=4004, reason=...) runs for all non-active sessions.

codeframe/ui/routers/terminal_ws.py (1)

146-148: Consider explicit guards instead of assert for production resilience.

The assert statements serve as type narrowing, but they can be disabled with python -O. Since these nested functions are created only after successful subprocess spawn, this is unlikely to cause issues in practice, but explicit guards would be more defensive:

♻️ Optional: Replace asserts with explicit guards

         async def _stdout_relay() -> None:
-            assert process is not None
-            assert process.stdout is not None
+            if process is None or process.stdout is None:
+                return
             try:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@codeframe/ui/routers/terminal_ws.py` around lines 146 - 148, The nested
function _stdout_relay currently uses assert process is not None and assert
process.stdout is not None; replace those asserts with explicit runtime guards:
check if process is None or process.stdout is None at the start of
_stdout_relay, log or raise a clear RuntimeError (or return early) with
contextual message including the function name, and ensure any callers handle
that error/path; reference the symbols _stdout_relay, process, and
process.stdout when making the change so the guard replaces the asserts and
provides deterministic behavior in production.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

codeframe/ui/routers/terminal_ws.py (2)

105-108: ⚠️ Potential issue | 🟠 Major

State check allows "paused" sessions to spawn terminals.

The condition only rejects "ended" sessions, but "paused" sessions should also be denied terminal access. Only "active" sessions should have an interactive terminal.

Proposed fix

     session = await asyncio.to_thread(db.interactive_sessions.get, session_id)
-    if session is None or session.get("state") == "ended":
+    if session is None or session.get("state") != "active":
         await websocket.close(code=4004, reason="Session not found or ended")
         return

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@codeframe/ui/routers/terminal_ws.py` around lines 105 - 108, The session
state check allows "paused" sessions to open terminals; update the condition
around the session fetched via db.interactive_sessions.get (variable session) to
only permit sessions with state == "active" (or explicitly deny states "paused"
and "ended") and call websocket.close(code=4004, reason="Session not found,
paused, or ended") when denied; adjust the check near the websocket handling
logic so any session.get("state") != "active" triggers the close.

---

110-114: ⚠️ Potential issue | 🔴 Critical

Critical: Ownership check is ineffective—any authenticated user can access any session's terminal.

The interactive_sessions table has no user_id column. When the ownership check attempts session.get("user_id") at line 111, it returns None, causing the conditional at line 112 to short-circuit and skip access denial. Combined with the fact that sessions are created without user association (as noted in interactive_sessions_v2.py), any authenticated user can connect to any session's terminal.

Use a fail-closed approach: deny access if ownership cannot be verified.

Proposed fix

     # --- Ownership check ---
     session_user_id = session.get("user_id")
-    if session_user_id is not None and int(session_user_id) != user_id:
+    if session_user_id is None:
+        logger.warning("Session %s has no user_id; denying terminal access", session_id)
+        await websocket.close(code=4003, reason="Session ownership cannot be verified")
+        return
+    if int(session_user_id) != user_id:
         await websocket.close(code=4003, reason="Forbidden: session belongs to another user")
         return

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@codeframe/ui/routers/terminal_ws.py` around lines 110 - 114, The ownership
check is currently ineffective because session.get("user_id") can be None;
update the logic to fail-closed: if session_user_id is None OR
int(session_user_id) != user_id then immediately call websocket.close(code=4003,
reason="Forbidden: session belongs to another user") and return. Additionally,
because the interactive_sessions table has no user_id, add a verification step
that queries the session owner from the authoritative source in
interactive_sessions_v2 (e.g., call the existing lookup/query function in
interactive_sessions_v2.py to retrieve the session owner by session_id) and
compare that owner to user_id; if that lookup yields no owner or a mismatch,
close the websocket. Ensure you reference the variables session_user_id,
session.get("user_id"), and the websocket.close(...) call when making the
change.

web-ui/src/components/sessions/AgentTerminal.tsx (1)

37-51: Consider using Hugeicons for consistency.

The coding guidelines specify using @hugeicons/react for icons. If Hugeicons has a suitable loading/spinner icon, consider using it for consistency with the rest of the UI.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/components/sessions/AgentTerminal.tsx` around lines 37 - 51,
Replace the inline SVG spinner in AgentTerminal.tsx with the Hugeicons spinner
component to match project conventions: import the appropriate spinner icon from
`@hugeicons/react` (e.g., Spinner or SpinnerSolid) and use that component in place
of the SVG block shown in the diff (the <svg ...> / </svg> spinner). Ensure you
pass the same sizing and color classes (e.g., animate-spin h-5 w-5
text-purple-400) to the Hugeicons component so appearance and animation remain
consistent.